Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Infrastructure Update to improve Testability #881

Merged
merged 5 commits into from
Mar 5, 2021
Merged

Infrastructure Update to improve Testability #881

merged 5 commits into from
Mar 5, 2021

Conversation

eugeniomiro
Copy link
Contributor

This PR adds the following features

  • Unity upgrade to 5.11.7
  • Coalesce all Unit Tests into one namespace
  • Test Framework Migration to MSTestV2
  • refactor ConfigurationEntry for Testability

@eugeniomiro
Copy link
Contributor Author

Hi @willdean, is the project still accepting contributions?
Is there anything I can do to improve this PR to make it acceptable?
I know you mentioned in my previous PR(#880) that there were too many changes on it but this is a smaller one, do you think I need to shrink it even more?

@willdean
Copy link
Collaborator

willdean commented Mar 5, 2021

Hi @eugeniomiro - sorry for not responding to this - I have not been thinking much about Bonobo recently.
I can take this - it only affects the tests.

As I side note, and it's not going to be a problem now, but I would say that #3d283cd shows a problem where VS and nuget disagree about XML formatting and hundreds of lines get meaningless white-space differences.

In theory, every single changed line has to be looked at by someone taking a contribution, so avoiding big whitespace changes is helpful. Obviously this is not your fault, it's VS stupidity, but just something to think about.

@willdean willdean merged commit 6277ecf into jakubgarfield:master Mar 5, 2021
@eugeniomiro eugeniomiro deleted the develop branch March 5, 2021 16:22
@eugeniomiro
Copy link
Contributor Author

Thank you @willdean, I took note of your comment about the XML in #3d283dc, will be more careful next time

@willdean
Copy link
Collaborator

willdean commented Mar 5, 2021

Many thanks for the contributions, I didn't say that last time.

@eugeniomiro
Copy link
Contributor Author

One more thing @willdean, I'm preparing the next PR with all those tests I sent in #880, trying not to touch the main code, that's in preparation for the fix of #882 which is the Issue I found and wanted to fix in the first place, but wanted to do it using a set of tests as a contention net. I have those in a large set of commits, is it best for you that I squash them all for the PR?
Any other suggestion for it?

Another thing, I'm adding .editorconfig as one of my files, to keep source code formatting consistency. I leave this comment to check if you want that in or not, I think it could be very useful.

@willdean
Copy link
Collaborator

willdean commented Mar 5, 2021

Hi @eugeniomiro I think it probably is easier if they're squashed, so long as they're all related to each other. One of the problems if they're not squashed is that the same files often appear in multiple commits, which makes a lot more work to review.

Personally I'm ok with adding .editorconfig as long as it keeps the style consistent with the existing code. That should probably be in a separate commit/PR though, because it's not really related to changing tests or fixing bugs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants